Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[RHOAIENG-7942]: Update Side Panel Behavior to Pan Graph on Node Selection #2943

Merged
merged 1 commit into from
Jul 10, 2024
Merged

[RHOAIENG-7942]: Update Side Panel Behavior to Pan Graph on Node Selection #2943

merged 1 commit into from
Jul 10, 2024

Conversation

jenny-s51
Copy link
Contributor

@jenny-s51 jenny-s51 commented Jun 21, 2024 •
edited
Loading

https://issues.redhat.com/browse/RHOAIENG-7942

Description

Previously if we selected a pipelines node and the side panel opened, the node that toggled the panel was obscured by the panel.

Now when a node is selected, the node that toggled the drawer is panned into view.

Screen.Recording.2024-06-21.at.2.45.49.PM.mov

How Has This Been Tested?

  1. Navigate to a pipeline, pipeline run, or scheduled run
  2. Click + drag the graph to the right of the topology view (such as to be obscured by the side panel opening on the right)
  3. Select a node in the pipeline
  4. Verify the node/graph pans to the left of the drawer panel and is not obscured by the panel

Test Impact

N/A

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@openshift-ci openshift-ci bot requested review from dpanshug and ppadti June 21, 2024 19:39
@openshift-merge-robot openshift-merge-robot added the needs-rebase PR needs to be rebased label Jun 21, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase PR needs to be rebased label Jun 24, 2024
@jenny-s51 jenny-s51 force-pushed the RHOAIENG-7942 branch 3 times, most recently from e36aec4 to ef67597 Compare June 27, 2024 19:48
@Gkrumbach07
Copy link
Member

Screenshot 2024-07-01 at 7 59 20 AM

@jenny-s51 It looks like the topology action buttons are pushed under the screen

@jeff-phillips-18
Copy link
Contributor

With out the setting of the min-width for the side panel, we can get:
image

@jeff-phillips-18
Copy link
Contributor

jeff-phillips-18 commented Jul 2, 2024 •
edited
Loading

@jenny-s51 It looks like the topology action buttons are pushed under the screen

This was the reason for:

  .pf-topology-container {
    overflow-y: hidden;

and the flex components in https://github.com/opendatahub-io/odh-dashboard/pull/2943/files#diff-153248b4472ef25aa4c01cf4f312bdddef50f7fe6052090df18ddd810437af90

@jenny-s51 jenny-s51 force-pushed the RHOAIENG-7942 branch 2 times, most recently from 64f9308 to 9758f52 Compare July 2, 2024 15:26
jenny-s51
jenny-s51 commented Jul 2, 2024
View reviewed changes
Copy link
Contributor Author

@jenny-s51 jenny-s51 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your PR feedback @jeff-phillips-18 . Reverted to apply the styles and Flex components

@Gkrumbach07
Copy link
Member

@jenny-s51 The drawer can be too small now and some tabs get hidden
Screenshot 2024-07-02 at 5 24 09 PM

@jenny-s51 jenny-s51 force-pushed the RHOAIENG-7942 branch 5 times, most recently from a65e3a2 to 54e8027 Compare July 3, 2024 19:54
@jenny-s51
Copy link
Contributor Author

jenny-s51 commented Jul 3, 2024 •
edited
Loading

@Gkrumbach07 while looking into the issue you found, I noticed a bug with the drawer component that was causing the drawer panel to remain in the view when the drawer was closed.
Screenshot 2024-07-03 at 11 30 23 AM

@jeff-phillips-18 helped come up with a good solution to this, updated the PR with the fix.

@Gkrumbach07 still seeing the issue with the hidden tabs though on initial resize. Investigating... this may be an issue on the PF side

@jeff-phillips-18
Copy link
Contributor

still seeing the issue with the hidden tabs though on initial resize. Investigating... this may be an issue on the PF side

Looks to be a PF issue. The Tabs component only determines if the scrolling components are needed when the window resizes. If the user resizes the side panel, the Tabs will not realize that the scroll components have become necessary.

@openshift-merge-robot openshift-merge-robot added the needs-rebase PR needs to be rebased label Jul 8, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase PR needs to be rebased label Jul 8, 2024
@jenny-s51 jenny-s51 force-pushed the RHOAIENG-7942 branch 2 times, most recently from 6a74c59 to 2b51cbd Compare July 9, 2024 13:29
@jenny-s51
Copy link
Contributor Author

Investigating test errors in pipelinesTopology with the latest rebase/updates (after these files were recently refactored on main). It appears that some styling is missing after the rebase. I'm currently looking into this - will implement the necessary fixes and update accordingly.

@jenny-s51 jenny-s51 force-pushed the RHOAIENG-7942 branch 3 times, most recently from f0cc1be to 0e4ddec Compare July 10, 2024 15:00
@jenny-s51
[RHOAIENG-7942]: Update Side Panel Behavior to Pan Graph on Node Sele…
a8c5d14 
…ction

Fix side panel sizing, scrolling, and contents (#8)

apply light variants to page section, fix linting issue

Gage PR feedback

fix tests

add flexItems and minWidth

Fix to hide side panel completely on close

update minWidth, fix pipelineRunDetails

fix pipeline details view

update PipelineRunDetailsTabs

try fixing test error
@jenny-s51
Copy link
Contributor Author

Updated/rebased with latest changes from main and fixed tests.

@jeff-phillips-18 @Gkrumbach07 if you have a moment to take another look would be much appreciated

Gkrumbach07
Gkrumbach07 approved these changes Jul 10, 2024
View reviewed changes
Copy link
Member

@Gkrumbach07 Gkrumbach07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jul 10, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jul 10, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Gkrumbach07

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit f4f6af9 into opendatahub-io:main Jul 10, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gkrumbach07 Gkrumbach07 Gkrumbach07 approved these changes

@yannnz yannnz Awaiting requested review from yannnz

@jeff-phillips-18 jeff-phillips-18 Awaiting requested review from jeff-phillips-18

Assignees

@Gkrumbach07 Gkrumbach07

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jenny-s51 @Gkrumbach07 @jeff-phillips-18 @openshift-merge-robot